-
Notifications
You must be signed in to change notification settings - Fork 542
[apiserver] Start apiserver v2 in apiserver/cmd/main.go #3603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Troy Chiu <[email protected]>
@rueian could you take a look? Thank you |
Signed-off-by: Troy Chiu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@kevin85421 PTAL thank you! |
Signed-off-by: Troy Chiu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chatted with @rueian yesterday. Our conclusions are:
- v1.4.0 is the last release where we fix bugs and add new features for v1.
- We want to launch both v1 and v2 at the same time so that users can incrementally migrate from v1 to v2, and don't need to be 100% one time.
- We don't want to create a new quay repository such as
kuberay/apiserverv2
. This may increase the complexity of migration. - For v1.5.0, the image ``kuberay/apiserver` will be v2 only.
apiserver/cmd/main.go
Outdated
@@ -144,8 +146,19 @@ func startHttpProxy() { | |||
registerHttpHandlerFromEndpoint(ctx, api.RegisterRayServeServiceHandlerFromEndpoint, "ServeService", runtimeMux) | |||
registerHttpHandlerFromEndpoint(ctx, api.RegisterRayJobSubmissionServiceHandlerFromEndpoint, "RayJobSubmissionService", runtimeMux) | |||
|
|||
kubernetesConfig, err := config.GetConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a feature gate (maybe a env var) for users to disable v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a flag instead, since that has been a pattern in api server
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
apiserver/go.mod
Outdated
@@ -28,6 +25,8 @@ require ( | |||
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 | |||
github.com/grpc-ecosystem/grpc-gateway/v2 v2.26.3 | |||
github.com/onsi/gomega v1.37.0 | |||
github.com/ray-project/kuberay/apiserversdk v0.0.0-20250519054718-f3ebea713a2f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since apiserversdk hasn't been released. I use the master commit instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that the apiserver image uses different commits for apiserver v1 and v2? If so, it may cause some very hard to debug issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fortunately #3640 solve this!
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
cc @kevin85421 |
@@ -133,6 +133,32 @@ rules: | |||
- get | |||
- list | |||
--- | |||
# apiserversdk requires the following permissions to be able to list and proxy services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to update Helm chart's RBAC? If you plan to open a follow up PR for Helm chart, can you open an issue to track the progress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we should. Thank you for catching this. Let me create an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #3648
apiserver/go.mod
Outdated
@@ -28,6 +25,8 @@ require ( | |||
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 | |||
github.com/grpc-ecosystem/grpc-gateway/v2 v2.26.3 | |||
github.com/onsi/gomega v1.37.0 | |||
github.com/ray-project/kuberay/apiserversdk v0.0.0-20250519054718-f3ebea713a2f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that the apiserver image uses different commits for apiserver v1 and v2? If so, it may cause some very hard to debug issues.
corev1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
||
"github.com/ray-project/kuberay/apiserver/test/e2e" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid importing apiserver
? The payoff will come when we start deprecating v1. I am OK to add a file to add some test utils which copy some functions from apiserver/test/e2e
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point. I copied some utilities to e2e/apiserversdk and removed the unnecessary parts.
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
cc @rueian would you mind reviewing another iteration? Thanks! |
Signed-off-by: Troy Chiu <[email protected]>
@troychiu please ping me when all CI tests pass. |
@kevin85421 they passed! Thank you! |
Why are these changes needed?
This PR start apiserver v2 in
apiserver/cmd/main.go
to ship the apiserver v2.Related issue number
Closes #3596
Checks